Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Numark-Mixtrack-3-scripts.js #14180

Open
wants to merge 6 commits into
base: 2.5
Choose a base branch
from

Conversation

endcredits33
Copy link

@endcredits33 endcredits33 commented Jan 16, 2025

Improve Browse knob/button behaviour by allowing focus shift between search, sidebar, and track table with Shift+turn Browse.

Further discussion: #14165

edit(@ronso0)

The below is what I've settled on and I think it works well and minimises usage of the Shift button to create a simpler mapping.

  • Turn: scroll through focused library element
  • Shift + turn: move focus between search/sidebar/track table
  • Press: expand sidebar item or load track (depending on focus)
  • Shift + press: minimise/maximise library

Improve Browse knob/button behaviour by allowing focus shift between search, sidebar, and track table with Shift+turn Browse.

Browse button expands sidebar items or loads track depending on focus. Shift+press Browse maximises the track table.

Further discussion: mixxxdj#14165
@ronso0
Copy link
Member

ronso0 commented Jan 18, 2025

Thank you, this makes sense.

FWIW Shift + turn may also be used to scroll the focused view, and Shift + press does move the focus, but you decided to maximize the library with that, which is fine given the limited buttons on the Mixtrack3.
However, this alternative mappin would be a good candidate for controller settings. If you're interested working on this (in another PR), let me know.

And the usual paperwork:
Please sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

@ronso0
Copy link
Member

ronso0 commented Jan 18, 2025

Btw mapping improvements and new mappings should target the 'stable' branch (currently 2.5), so please rebase onto 2.5.

@endcredits33 endcredits33 changed the base branch from main to 2.5 January 18, 2025 21:59
@endcredits33
Copy link
Author

Thank you - rebased onto 2.5, agreement signed and made the requested update to BrowseKnob as well

@ronso0
Copy link
Member

ronso0 commented Jan 23, 2025

Please check endcredits33#2

ronso0 and others added 3 commits January 24, 2025 02:02
Mixtrack3: add option for Library navigation mode: Focus | Classic
Fix libraryModes reference to focus mode
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you, couple nitpicks

Comment on lines +23 to +24
In Classic mode the Browse encoder can select items in the sidebar and tracks table only, but independent
of keyboadr focus.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In Classic mode the Browse encoder can select items in the sidebar and tracks table only, but independent
of keyboadr focus.
In Classic mode the Browse encoder can select items in the sidebar and tracks table only, but independent
of keyboard focus.

Comment on lines +192 to +197
const libraryModes = {
focus: 0,
classic: 1,
};
const LibraryMode = libraryModes[engine.getSetting("libraryMode")] || libraryModes.focus;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove this part entirely if you do the below instead

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit of querying the settings each time compared to this const?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simplicity. As long as its not done in a tight loop, querying the setting should not be expensive. Doing all of this mapping is unnecessarily weird IMO. Additionally, global consts in mappings are kind off iffy (can suddenly break other things due to the way JS modules work, so thats why its always been encouraged to only add properties to the controller object instead of the global one).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmokay.
tight loop = called at high frequency?

So NumarkMixtrack3.libraryMode = engine.getSetting().. would be okay?
And settings is not an object of NumarkMixtrack3 here, right?

Copy link
Member

@Swiftb0y Swiftb0y Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tight loop = called at high frequency?

Yes.

So NumarkMixtrack3.libraryMode = engine.getSetting().. would be okay?

Yes, better than the current one... But I won't insist on this.

script.toggleControl("[Skin]", "show_maximized_library");
}
}
if (LibraryMode === libraryModes.focus) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (LibraryMode === libraryModes.focus) {
if (engine.getSetting("libraryMode") === "focus") {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants